-
Notifications
You must be signed in to change notification settings - Fork 30
Functions api ✨ 🗃️ #7539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Functions api ✨ 🗃️ #7539
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7539 +/- ##
==========================================
- Coverage 87.57% 86.03% -1.54%
==========================================
Files 1794 1731 -63
Lines 69368 68182 -1186
Branches 1136 1037 -99
==========================================
- Hits 60747 58661 -2086
- Misses 8311 9247 +936
+ Partials 310 274 -36
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the great effort Werner! It looks very cool.
I suggest several minor changes, but I have one big request and that concerns running the function. I think it is crucial that that happens asyncronously and in one of our dedicated celery workers. This call (in the case of studies) was exactly what was blocking us last time we did the metamodeling. We are in a better position now because we have a job scheduling framework for handling "internal long running tasks". We should use it! 😁
packages/postgres-database/src/simcore_postgres_database/models/functions_models_db.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/functions_models_db.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/functions_models_db.py
Outdated
Show resolved
Hide resolved
...ages/service-library/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions.py
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
Yeah, sure makes sense. I'm basically calling the study_job api. It's probably best to implement the celery changes there? (it they're not there yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the changes. I had another look and provided some more feedback. I think it is quite critical that we sort out the run
function so that creating it becomes a long running task in a celery worker somewhere.
packages/models-library/src/models_library/api_schemas_api_server/functions.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_api_server/functions.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/functions/_functions_repository.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the Functions API to support registering, executing, and managing functions (across projects, solvers, and Python code) as well as caching their execution results. Key changes include the removal of the legacy webserver RPC controller, the addition of new API endpoints and service methods for functions handling, and the introduction of new schema models and database migrations for functions, function jobs, and their collections.
Reviewed Changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
services/web/server/src/simcore_service_webserver/functions/_controller_rpc.py | Removed the legacy RPC controller for functions since the functionality has been migrated. |
services/api-server/tests/unit/api_functions/conftest.py | Added new test fixtures and mocks to support the Functions API. |
services/api-server/src/simcore_service_api_server/services_rpc/wb_api_server.py | Extended the API client with methods for registering, getting, and managing functions and function jobs. |
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py | Introduced new schema models for the Functions API. |
services/api-server/src/simcore_service_api_server/api/routes/studies_jobs.py | Refactored study job routes for clearer control flow and direct returns. |
services/api-server/src/simcore_service_api_server/api/routes/solvers_jobs.py | Minor update with a noqa flag for improved linter compliance. |
services/api-server/src/simcore_service_api_server/api/root.py | Updated the API router to include new endpoints with dedicated prefixes for functions, function jobs, and function job collections. |
packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py | Added a new RPC interface for functions, exposing methods for all functions-related operations. |
packages/postgres-database/... | Added new database models and migrations to support functions, function jobs, and their collections. |
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py | Added new models and type aliases for the functions API exposed via the webserver. |
Files not reviewed (2)
- services/api-server/requirements/_test.in: Language not supported
- services/api-server/requirements/_test.txt: Language not supported
Comments suppressed due to low confidence (1)
services/api-server/tests/unit/api_functions/conftest.py:299
- [nitpick] Consider simplifying or aliasing the long module path used in the patch calls to improve readability and maintainability.
mock_wb_api_server_rpc.patch("servicelib.rabbitmq.rpc_interfaces.webserver.functions.functions_rpc_interface.register_function", backend_function_register.register_function)
packages/postgres-database/src/simcore_postgres_database/models/funcapi_function_jobs_table.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Let's get thisone in . but first, please check my suggestions and consider some changes.
If you have any questions, do not hesitate to drop be a line so we accelerate this PR merging.
TIP: activate mergify bot (write @mergify queue
) so that once all tests pass, it makes sure that it merges.
...gres-database/src/simcore_postgres_database/models/funcapi_function_job_collections_table.py
Show resolved
Hide resolved
...gres-database/src/simcore_postgres_database/models/funcapi_function_job_collections_table.py
Outdated
Show resolved
Hide resolved
...gres-database/src/simcore_postgres_database/models/funcapi_function_job_collections_table.py
Outdated
Show resolved
Hide resolved
.../simcore_postgres_database/models/funcapi_function_job_collections_to_function_jobs_table.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/api/routes/functions_routes.py
Show resolved
Hide resolved
services/api-server/tests/unit/api_functions/test_api_routers_functions.py
Outdated
Show resolved
Hide resolved
services/api-server/tests/unit/api_functions/test_api_routers_functions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few more comments and suggestions.
Also a way to simplify the api-server's tests which will remove quite some code and make them more intuitive when reading.
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Outdated
Show resolved
Hide resolved
...core_postgres_database/migration/versions/32a9ab5fa107_add_creation_modification_time_to_.py
Outdated
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
services/api-server/tests/unit/api_functions/test_api_routers_functions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Enables the new Functions API end-to-end by removing legacy stubs, adding RPC client wrappers, Pydantic schemas, HTTP routes, database tables, and migrations.
- Removes old webserver RPC controller for functions.
- Adds RPC client methods in
WbApiRpcClient
and Pydantic schemas for functions, jobs, and collections. - Registers new
/functions
,/function_jobs
, and/function_job_collections
routes and creates SQL tables with migrations.
Reviewed Changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
services/web/server/src/simcore_service_webserver/functions/_controller_rpc.py | Removed old RPC router and ping endpoint |
services/api-server/tests/unit/api_functions/conftest.py | Added fixtures and dummy RPC client for functions API tests |
services/api-server/src/simcore_service_api_server/services_rpc/wb_api_server.py | Added wrapper methods for functions RPC in WbApiRpcClient |
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py | New Pydantic schemas for functions, jobs, and collections |
services/api-server/src/simcore_service_api_server/api/root.py | Registered new routers for functions, jobs, and collections |
packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/ | RPC interface implementations for all functions endpoints |
packages/postgres-database/src/simcore_postgres_database/models/funcapi_*.py | Added tables for functions, jobs, collections, and join |
packages/postgres-database/src/simcore_postgres_database/migration/versions/*.py | Alembic scripts to create and evolve function API tables |
Files not reviewed (2)
- services/api-server/requirements/_test.in: Language not supported
- services/api-server/requirements/_test.txt: Language not supported
Comments suppressed due to low confidence (2)
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py:74
- [nitpick] The
FunctionJobCollection
model usesid
for its primary key field while other models useuid
; consider renamingid
touid
for consistency across your schemas.
class FunctionJobCollection(BaseModel):
services/api-server/src/simcore_service_api_server/services_rpc/wb_api_server.py:3
- The imports
dataclass
andpartial
are unused in this file; please remove them to clean up dependencies.
from dataclasses import dataclass
.../simcore_postgres_database/models/funcapi_function_job_collections_to_function_jobs_table.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry didn't have time to go over it, but I do not want to block you so approve, I will go over it as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I added a few comments.
For next time please create smaller PRs that are easier to review (you will also get faster reviews):
- 1 PR with database changes
- 1 PR with API changes,
- ...
Thank you and good luck!
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_webserver/functions_wb_schema.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/funcapi_function_jobs_table.py
Outdated
Show resolved
Hide resolved
packages/postgres-database/src/simcore_postgres_database/models/funcapi_functions_table.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the changes!
I'm OK with it, just some minor things still remain.
...ibrary/src/servicelib/rabbitmq/rpc_interfaces/webserver/functions/functions_rpc_interface.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/models/schemas/functions_api_schema.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/04/functions_rpc/test_functions_controller_rpc.py
Outdated
Show resolved
Hide resolved
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 67c3d67 |
What do these changes do?
This introduces the Functions API.
Functions are objects that can be in the backend: projects, solvers, python code, etc ...
Users can create their own functions from the relevant objects.
They accept parameters and return results.
Each function run creates a function job. These function job can also act as a cache to prevent rerunning a function with the same parameters.
Related issue/s
#7506
How to test
There are tests in the api-server (Rest) and web/server (RPC) service directories.
A high-level functional test script is here:
https://github.com/wvangeit/osparc-functions-api-test/blob/master/test.py
Dev-ops checklist